charts/matomo/templates/cronjob-archiver.yaml (1)

44-45: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift

Same host:port concatenation issue as deployment.yaml.

This duplicates the MATOMO_DATABASE_HOST construction flagged in charts/matomo/templates/deployment.yaml (lines 72-73); the archiver job will fail to connect to the database for the same reason if a non-default port is configured. Fixing this in one place (e.g., a shared helper) would avoid the two definitions drifting.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/matomo/templates/cronjob-archiver.yaml` around lines 44 - 45, The
MATOMO_DATABASE_HOST value in the cronjob archiver template repeats the same
broken host:port concatenation used elsewhere, so it should be updated to use
the same corrected database host formatting as deployment.yaml. Fix the
construction in the archiver’s MATOMO_DATABASE_HOST entry by reusing a shared
helper or the same logic used by the deployment template, and ensure only the
host is used where the container expects a host value. Reference the
MATOMO_DATABASE_HOST setting and the matomo.databaseHost/matomo.databasePort
helpers to keep both templates consistent.

charts/matomo/templates/networkpolicy.yaml (1)

29-47: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Database egress rule allows traffic to any destination on the DB port.

The egress rule at Lines 38-40 restricts only by port, not destination, since there's no to selector. Combined with extraTo being optional, workloads can reach any external host on the database port by default, weakening the isolation NetworkPolicy is meant to provide.

Consider requiring a to selector (via a configurable external DB CIDR/selector value) or documenting this permissive default so operators know to lock it down for production.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/matomo/templates/networkpolicy.yaml` around lines 29 - 47, The egress
rule in the network policy template is too permissive because the database port
rule in the egress block under the networkPolicy.egress.enabled path has no
destination selector. Update the matomo networkpolicy.yaml logic around the
database egress entry to require a configurable `to` target (for example via a
DB CIDR/selector value) or otherwise make the default explicit and documented;
keep the existing `extraTo` and `extraEgress` handling but ensure the main
database egress rule is not open to any destination by default.

charts/matomo/templates/servicemonitor.yaml (1)

18-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Hardcoded metrics path — consider making configurable.

Path /matomo.php is hardcoded rather than sourced from values, unlike the ClickHouse chart's .Values.metrics.path pattern shown in the snippet. If Matomo's metrics/health endpoint could change or is user-configurable (e.g., a subdirectory install), this locks it in.

♻️ Suggested change (verify a matching values.schema.json / values.yaml key exists first)

   endpoints:
     - port: http
-      path: /matomo.php
+      path: {{ .Values.metrics.path | default "/matomo.php" | quote }}
       interval: {{ .Values.metrics.serviceMonitor.interval }}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/matomo/templates/servicemonitor.yaml` around lines 18 - 19, The
ServiceMonitor in the Matomo chart hardcodes the probe path, so update the
servicemonitor template to read the endpoint from chart values instead of using
/matomo.php directly. Use the existing values-driven pattern from the chart’s
templating (for example a metrics path value in values.yaml/values.schema.json)
and reference the same key from the ServiceMonitor resource so users can
override it for custom installs or subdirectory setups.

charts/matomo/templates/extra-manifests.yaml (1)

1-6: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Inconsistent with the generic extra-manifests pattern.

The charts/generic/templates/extra-manifests.yaml reference implementation guards against a nil list (| default list) and pipes through tpl to allow templated values (e.g., {{ .Release.Name }}) inside user-supplied manifests. This version omits both, so extra manifests here can't reference chart values/templates and relies on implicit nil-range safety.

♻️ Align with the canonical generic template

 {{/* SPDX-License-Identifier: Apache-2.0 */}}
-{{- range .Values.extraManifests }}
+{{- range .Values.extraManifests | default list }}
 ---
-{{ toYaml . }}
+{{ tpl (toYaml .) $ }}
 {{- end }}

Based on cross-file evidence from `charts/generic/templates/extra-manifests.yaml`, which uses `{{- range .Values.extraManifests | default list }}` and `{{ tpl (toYaml .) $ }}`.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/matomo/templates/extra-manifests.yaml` around lines 1 - 6, The
extra-manifests template is missing the canonical safeguards and templating
support used by the generic chart. Update the extra-manifests loop in the
templates/extra-manifests.yaml logic to default .Values.extraManifests to an
empty list before ranging, and render each manifest through tpl after toYaml so
user-supplied manifests can reference chart values like .Release.Name. Keep the
existing behavior aligned with the generic extra-manifests pattern.

charts/matomo/tests/routing_test.yaml (1)

3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing default-off test for HTTPRoute.

The suite verifies ingress doesn't render by default but has no equivalent assertion for gateway-httproute.yaml, even though it's included in templates.

Suggested addition

   - it: does not render ingress by default
     template: ingress.yaml
     asserts:
       - hasDocuments:
           count: 0
+  - it: does not render HTTPRoute by default
+    template: gateway-httproute.yaml
+    asserts:
+      - hasDocuments:
+          count: 0

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/matomo/tests/routing_test.yaml` around lines 3 - 5, The routing test
suite currently checks the default-off behavior for ingress but does not assert
the same for gateway-httproute.yaml even though it is listed in templates.
Update the routing test fixture and assertions in routing_test.yaml to
explicitly verify that the HTTPRoute template does not render by default, using
the gateway-httproute.yaml template entry as the target to locate the missing
coverage.

charts/clickhouse/tests/validation_test.yaml (1)

5-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for the remaining clickhouse.validate branches.

Good coverage overall, but two fail branches in charts/clickhouse/templates/_helpers.tpl (Lines 94, 99) aren't exercised:

• replicaCount: 0 (the lt ... 1 branch, distinct from the gt ... 1 branch already tested at Line 6-11)
• podLabels overriding app.kubernetes.io/name (only app.kubernetes.io/instance is tested at Lines 12-18)

Since these are hard validation gates that block chart installs, missing coverage risks silent regressions if the fail conditions are ever refactored.

✅ Suggested additional test cases

+  - it: blocks replicaCount below minimum
+    set:
+      replicaCount: 0
+    asserts:
+      - failedTemplate:
+          errorMessage: "replicaCount must be at least 1"
+  - it: blocks name label override
+    set:
+      podLabels:
+        app.kubernetes.io/name: bad
+    asserts:
+      - failedTemplate:
+          errorMessage: podLabels must not override app.kubernetes.io/name

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/clickhouse/tests/validation_test.yaml` around lines 5 - 43, Add test
coverage for the remaining clickhouse.validate failure branches in the
validation suite. Extend the tests in validation_test.yaml to exercise the
`clickhouse.validate` logic in `_helpers.tpl` by adding a case for
`replicaCount: 0` to hit the `lt ... 1` path, and another case where `podLabels`
overrides `app.kubernetes.io/name` to verify that selector label override is
rejected. Use the existing `failedTemplate` pattern alongside the current
`replicaCount`, `podLabels`, and `clickhouse.validate` checks.

charts/matomo/values.yaml (1)

217-219: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

No default securityContext/podSecurityContext hardening.

Both securityContext and podSecurityContext default to empty objects, so the Matomo Apache container runs without runAsNonRoot, dropped capabilities, or a read-only root filesystem by default. Given this chart targets production use (per DESIGN/README goals), consider shipping sane secure defaults (e.g., runAsNonRoot: true, allowPrivilegeEscalation: false, capabilities.drop: [ALL]), while leaving them overridable.

Example defaults

-podSecurityContext: {}
-
-securityContext: {}
+podSecurityContext:
+  fsGroup: 33
+
+securityContext:
+  allowPrivilegeEscalation: false
+  capabilities:
+    drop: ["ALL"]

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/matomo/values.yaml` around lines 217 - 219, The Matomo chart defaults
leave both securityContext and podSecurityContext empty, so the Apache container
starts without basic hardening. Update the default values in the values.yaml
entries for podSecurityContext and securityContext to include secure baseline
settings such as runAsNonRoot, allowPrivilegeEscalation disabled, and dropped
capabilities, while keeping them configurable for users who need different
settings.

charts/matomo/tests/validation_test.yaml (1)

5-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding test cases for the remaining matomo.validate branches.

Current suite covers only 3 of the ~7 fail branches in matomo.validate (podLabels override, ingress hosts, externalSecrets items). Missing coverage: gatewayAPI.httpRoutes empty, metrics.serviceMonitor.interval missing, external-DB-without-password, and externalSecrets.items[].spec missing secretStoreRef/data/dataFrom.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/matomo/tests/validation_test.yaml` around lines 5 - 28, Add validation
tests to cover the remaining failing branches in matomo.validate, since the
current suite only exercises a subset of the guards. Extend validation_test.yaml
with cases for gatewayAPI.httpRoutes being empty,
metrics.serviceMonitor.interval being omitted, external DB configured without a
password, and externalSecrets.items[].spec missing secretStoreRef, data, or
dataFrom. Use the existing failedTemplate pattern in the matomo.validate test
file so each branch is asserted by its exact errorMessage.

charts/matomo/templates/cronjob-archiver.yaml (1)

41-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated DB connection env block.

The MATOMO_DATABASE_* env definitions here are a near-exact copy of deployment.yaml's block (lines 72-82). Extracting a shared _helpers.tpl template (e.g., matomo.databaseEnv) for the env list would reduce drift risk between the Deployment and CronJob — directly relevant given the host/port bug above exists in both copies today.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/matomo/templates/cronjob-archiver.yaml` around lines 41 - 54, The
`MATOMO_DATABASE_*` environment block in the CronJob is duplicated from the
Deployment, so it should be centralized to avoid drift. Extract the shared env
list into a reusable helper in `_helpers.tpl` (for example, a
`matomo.databaseEnv` helper) and update both `cronjob-archiver.yaml` and
`deployment.yaml` to include that helper instead of maintaining separate copies.
Use the existing `MATOMO_DATABASE_HOST`, `MATOMO_DATABASE_USERNAME`,
`MATOMO_DATABASE_DBNAME`, and `MATOMO_DATABASE_PASSWORD` definitions as the
symbols to consolidate.

charts/clickhouse/tests/service_test.yaml (1)

10-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider adding coverage for the conditional metrics port.

Tests cover http/tcp ports but not the metrics port that's conditionally rendered when .Values.metrics.enabled is true.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/clickhouse/tests/service_test.yaml` around lines 10 - 20, Add test
coverage for the conditionally rendered metrics port in the ClickHouse service
tests. Update the existing service rendering assertions in service_test.yaml,
and/or add a new case that enables .Values.metrics.enabled so the rendered
Service can be checked for the metrics port alongside the existing http and tcp
ports. Use the service template’s port fields as the anchor for verification so
the test clearly covers the conditional behavior.

charts/clickhouse/templates/pdb.yaml (1)

2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

PDB condition is unreachable given the chart's single-replica constraint.

clickhouse.validate fails the render whenever replicaCount > 1, so the gt (int .Values.replicaCount) 1 guard here can never be true - this PDB template will never render under the chart's supported configuration. Since podDisruptionBudget.enabled: true is the default, consider clarifying in docs/values comments that PDB is a no-op for this single-node chart, or removing the dead conditional.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/clickhouse/templates/pdb.yaml` at line 2, The PDB guard in the
clickhouse PDB template is dead code because clickhouse.validate already rejects
any replicaCount greater than 1, so this branch can never render. Update the
chart behavior by either removing the unreachable gt (int .Values.replicaCount)
1 check from the podDisruptionBudget template or clarifying in the values/docs
for podDisruptionBudget.enabled that it is a no-op for this single-replica
chart; use clickhouse.validate and the PDB template condition as the symbols to
locate the fix.

charts/clickhouse/tests/pdb_test.yaml (1)

1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Only the "no PDB rendered" branch is tested.

The suite verifies pdb.yaml renders zero documents in the default/standalone case, but there's no assertion covering the positive path (podDisruptionBudget.enabled: true and replicaCount > 1), which would validate minAvailable/maxUnavailable selection logic in the template.

Suggested additional test case

 suite: clickhouse pdb
 templates:
   - pdb.yaml
 tests:
   - it: does not render for standalone default
     asserts:
       - hasDocuments:
           count: 0
+  - it: renders when enabled with multiple replicas
+    set:
+      replicaCount: 3
+      podDisruptionBudget:
+        enabled: true
+        minAvailable: 1
+    asserts:
+      - hasDocuments:
+          count: 1
+      - equal:
+          path: spec.minAvailable
+          value: 1

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/clickhouse/tests/pdb_test.yaml` around lines 1 - 9, The clickhouse PDB
test suite only covers the no-render case, so add a positive test for the
pdb.yaml template when podDisruptionBudget.enabled is true and replicaCount is
greater than 1. Use the existing pdb.yaml suite and its assertions to verify a
document is rendered and that the template selects the expected minAvailable or
maxUnavailable branch. Reference the pdb.yaml template and the current
standalone default test case when adding the new assertion.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

charts/matomo/templates/networkpolicy.yaml (1)

38-44: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Database egress rule allows any destination on that port.

The database egress rule has no to: selector, so it permits egress to the database port on any destination (not just the actual DB). This weakens the egress restriction's value — extraTo is optional, so by default nothing constrains the destination. Consider documenting this limitation clearly (e.g., in DESIGN.md/README) and recommending users populate networkPolicy.egress.extraTo for stricter isolation.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@charts/matomo/templates/networkpolicy.yaml` around lines 38 - 44, The
NetworkPolicy egress rule for the database port in the matomo chart currently
allows traffic to any destination because it has ports without a destination
selector. Update the chart documentation/comments around the network policy
handling to clearly state this limitation and point users to configure
networkPolicy.egress.extraTo for stricter isolation; reference the networkpolicy
template and the egress.extraTo values path so the guidance is easy to find even
if the template changes.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes